Update endpoints watcher to not fetch pods for removed endpoints#10013
Merged
Update endpoints watcher to not fetch pods for removed endpoints#10013
Conversation
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
alpeb
approved these changes
Dec 20, 2022
| } | ||
| } | ||
|
|
||
| // Test that when an EndpointSlice is scaled down, the EndpointsWatcher sends |
Member
There was a problem hiding this comment.
nit: godoc should start with the function name
Member
Author
There was a problem hiding this comment.
I don't think we typically use godoc for tests, do we? (note the // rather than ///)
Member
There was a problem hiding this comment.
Ok never mind, I thought one could use go doc to show docs from test functions, but that doesn't appear to be the case. OTOH, I'm not aware of the usage of ///, can you clarify?
Member
Author
There was a problem hiding this comment.
Nevermind, /// isn't a go thing. I got confused; holiday brain.
Signed-off-by: Alex Leong <alex@buoyant.io>
Contributor
|
@adleong Nice fix! It took me some time to digest why In particular, I found it unclear why one method would be leaky while the other isn't. Will you please update the comment(s) to avoid future confusion? |
hawkw
pushed a commit
that referenced
this pull request
Feb 3, 2023
) Fixes #10003 When endpoints are removed from an EndpointSlice resource, the destination controller builds a list of addresses to remove. However, if any of the removed endpoints have a Pod as their targetRef, we will attempt to fetch that pod to build the address to remove. If that pod has already been removed from the informer cache, this will fail and the endpoint will be skipped in the list of endpoints to be removed. This results in stale endpoints being stuck in the address set and never being removed. We update the endpoint watcher to construct only a list of endpoint IDs for endpoints to remove, rather than fetching the entire pod object. Since we no longer attempt to fetch the pod, this operation is now infallible and endpoints will no longer be skipped during removal. We also add a `TestEndpointSliceScaleDown` test to exercise this. Signed-off-by: Alex Leong <alex@buoyant.io>
hawkw
pushed a commit
that referenced
this pull request
Feb 6, 2023
) Fixes #10003 When endpoints are removed from an EndpointSlice resource, the destination controller builds a list of addresses to remove. However, if any of the removed endpoints have a Pod as their targetRef, we will attempt to fetch that pod to build the address to remove. If that pod has already been removed from the informer cache, this will fail and the endpoint will be skipped in the list of endpoints to be removed. This results in stale endpoints being stuck in the address set and never being removed. We update the endpoint watcher to construct only a list of endpoint IDs for endpoints to remove, rather than fetching the entire pod object. Since we no longer attempt to fetch the pod, this operation is now infallible and endpoints will no longer be skipped during removal. We also add a `TestEndpointSliceScaleDown` test to exercise this. Signed-off-by: Alex Leong <alex@buoyant.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #10003
When endpoints are removed from an EndpointSlice resource, the destination controller builds a list of addresses to remove. However, if any of the removed endpoints have a Pod as their targetRef, we will attempt to fetch that pod to build the address to remove. If that pod has already been removed from the informer cache, this will fail and the endpoint will be skipped in the list of endpoints to be removed. This results in stale endpoints being stuck in the address set and never being removed.
We update the endpoint watcher to construct only a list of endpoint IDs for endpoints to remove, rather than fetching the entire pod object. Since we no longer attempt to fetch the pod, this operation is now infallible and endpoints will no longer be skipped during removal.
We also add a
TestEndpointSliceScaleDowntest to exercise this.